I forgot to add. Here is how you'd use the API:

// direct connection, paired, reconnecting

use redis_async::client::{RedisConnectionBuilder, paired::PairedConnectionBuilder};

let client = RedisConnectionBuilder::new("url")
    .username("hello")
    .password("world")
    .paired_reconnecting()
    .await
    .unwrap();

let response = client.send(data).await.unwrap();

// connecting through sentinel, pubsub, not reconnecting

use redis_async::client::{SentinelConnectionBuilder, pubsub::PubsubConnectionBuilder};

let client = SentinelConnectionBuilder::new(vec!["host 1", "host 2"], "master_name")
    .redis_username("foo")
    .redis_password("bar")
    .pubsub_connect()
    .await
    .unwrap();

let stream = client.subscribe("message").await.unwrap();

Hello,

Thanks for this 👍 This is quite a big PR so I'll some more comments later once I have the chance to go in to some of the details in a bit more detail.

But some high-level comments:

Regarding reconnection - I think I would prefer if the default path maintained backward compatibility, just to reduce the risk of users not noticing the modified behaviour. But I could be persuaded otherwise if non-reconnecting clients was the idiom, but I think most Redis clients I've used for other programming languages have automatically handled reconnection. So on balance, I think that should probably be the default and non-reconnecting made the option.

I do like the new reconnection logic. It's much cleaner than what was there before, which grew organically for several versions and is currently quite gnarly.

One thing I'm not sure about how the library should behave in future is the send method on PairedConnect. Currently it's implemented in such a way so that the precise ordering of messages sent to Redis are the same as the order that send is called. This goes against the Rust idioms of futures doing nothing unless they're polled, but it did have two benefits: One, it meant that there wouldn't be any surprises if a future wasn't polled for whatever reason, so this would still work, even though the first future is thrown away without being .awaited on. It means we can also .await them in a different order if it makes sense to or even accidentally.

let set_future = con.send(resp_array!["SET", "x", "12"]);
let get_result = con.send(resp_array!["GET", "x"]).await;

Secondly, it enabled the implicit pipelining to occur (as described in the project's README). So I'm not sure where we should go with this. On the one hand, by making send a more classically async function we have something that's more idiomatic, but it could result in unsuspecting performance regressions in users of the library. So I need to have a bit of a think. This has been an issue for a while, but changes required for this PR might force the issue. In order to limit the size of this PR it might be something I need to fix in a separate PR which leapfrogs this one (possibly, I still need to think through the consequences).

Is error channel a good way of informing Reconnect about errors? Maybe we should wrap RespConnection with some struct that spies on the tcp channel and and pass that to complex connection? There might be a use case for having that level control in complex connection.

My first thought for this one is it should be fail-safe in the sense that we don't want it to be possible for the state the be NotConnected without a reconnection attempt being triggered. It looks like this is the case, we should see errors if anything went wrong.

I'll check out the PR locally in more detail over the next few days as time allows.

Thanks again 👍

I think I would prefer if the default path maintained backward compatibility, just to reduce the risk of users not noticing the modified behaviour. But I could be persuaded otherwise if non-reconnecting clients was the idiom, but I think most Redis clients I've used for other programming languages have automatically handled reconnection. So on balance, I think that should probably be the default and non-reconnecting made the option.

Agree. Let's not confuse people.

Secondly, it enabled the implicit pipelining to occur (as described in the project's README).

Do you mean sending multiple messages without waiting for responses? Like this?

client        redis

request A >
request B >
request C >

some time passes...

       < response A
       < response B
       < response C

I believe this still works, but you have to fire multiple futures at once. It might be even more intuitive, if you compare it to sleep for example:

let sleep_1 = tokio::time::sleep(Duration::from_secs(1));
let sleep_2 = tokio::time::sleep(Duration::from_secs(2));
let sleep_3 = tokio::time::sleep(Duration::from_secs(3));

sleep_1.await; // takes 1 second
sleep_2.await; // takes 2 seconds
sleep_3.await; // takes 3 seconds
               // 6 seconds in total 

// vs

let sleeps = vec![
    tokio::time::sleep(Duration::from_secs(1));
    tokio::time::sleep(Duration::from_secs(2));
    tokio::time::sleep(Duration::from_secs(3));
];

let _ = future::join_all(sleeps).await; // takes 3 seconds

My first thought for this one is it should be fail-safe in the sense that we don't want it to be possible for the state the be NotConnected without a reconnection attempt being triggered. It looks like this is the case, we should see errors if anything went wrong.

My thinking here was that we'll probably want to detect disconnects in paired connect anyway to cancel/fail request futures. And if we're doing that, it would be really easy to notify reconnect. In addition, complex connections could trigger reconnect if they find themselves in some unrecoverable state.